-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Upgrade to wgpu 24 #436
base: main
Are you sure you want to change the base?
Upgrade to wgpu 24 #436
Conversation
ArthurBrussee
commented
Jan 19, 2025
- Add atomic floats feature matrix to wgpu. In the future when f16 etc. are supported on wgpu this feature matrix might need to be more granular.
- Change checked execution - infinite loops are now again checked in checked mode as they can cause UB. It might make sense to make more granular options for this later.
crates/cubecl-wgpu/src/runtime.rs
Outdated
if features.contains(wgpu::Features::SHADER_FLOAT32_ATOMIC) { | ||
device_props.register_feature(Feature::AtomicFloat(AtomicFeature::LoadStore)); | ||
device_props.register_feature(Feature::AtomicFloat(AtomicFeature::Add)); | ||
device_props.register_feature(Feature::AtomicFloat(AtomicFeature::MinMax)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way I've implemented the features for float atomics, AtomicFeature
should be combined with registering the relevant atomic types. As far as I can tell, if an atomic type is supported at all then all operations are supported on it.
So this should also register Feature::Type(Elem::AtomicFloat(FloatKind::F32))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, as far as I can tell MinMax
isn't supported for float at least not universally, it isn't available in Vulkan on my card either, and CUDA doesn't have support for it at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR feature matrix for float atomics says it's supported in Vulkan (but not on the 3080), but not implemented at all in wgpu. So it definitely should be disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah bad memory really thought the PR said it was supported, nvm.
And I see about the Elem::AtomicFloat feature, that makes sense! Thank you
) -> Arc<ComputePipeline> { | ||
let source = &kernel.source; | ||
// Cube always in principle uses unchecked modules. Certain operations like | ||
// indexing are instead checked by cube. The WebGPU specification only makes | ||
// incredibly loose guarantees that Cube can't rely on. Additionally, kernels | ||
// can opt in/out per operation whether checks should be performed which can be faster. | ||
// | ||
let checks = ShaderRuntimeChecks { | ||
bounds_checks: false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a comment that automatic bounds checks are done by the cubecl compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above said as much but clarified this a bit